algod: Consolidate models #4714

Eric-Warehime · 2022-10-31T19:53:35Z

Summary

This PR builds on top of the oapi-codegen update

Whereas the previous PR adds a config file to do the code generation for each of the generated/routes.go, generated/types.go, generated/private/routes.go, and generated/private/types.go files, this PR moves the generated routes into a the public package, and combines both types files into a single model package.

The rest of the changes are as a result of this refactoring--changing imports.
generated=>model
generatedV2=>model
generated=>public in the case of the router

Test Plan

Existing tests provide coverage for all implemented APIs.

Updates the version of oapi-codegen which is installed via the buildtools to our v1.11.0 prerelease. Also changes the Makefile targets and adds config files which are the preferred way of configuration instead of the old CLI flags. The generated types definitions have also been updated using the oapi-codegen commit which ensures all required Enums are generated for enums embedded in response types.

…port-changes

Moves all types.go to a single model package within the v2 generated API code. Also moves the oapi-codegen config file for each of the generated packages alongside the code it is generating. Additionally, we've moved the generated routes.go into a public package instead of the top level generated package. The result of this is a large refactoring of imports. generated=>models, generatedV2=>models, and generated=>public in the case of the router.

codecov · 2022-10-31T20:18:18Z

Codecov Report

Merging #4714 (ea80a8b) into master (445b45b) will decrease coverage by 0.02%.
The diff coverage is 27.06%.

@@            Coverage Diff             @@
##           master    #4714      +/-   ##
==========================================
- Coverage   54.69%   54.67%   -0.03%     
==========================================
  Files         414      414              
  Lines       53548    53550       +2     
==========================================
- Hits        29290    29277      -13     
- Misses      21832    21844      +12     
- Partials     2426     2429       +3

Impacted Files	Coverage Δ
cmd/goal/account.go	`13.76% <0.00%> (ø)`
cmd/goal/accountsList.go	`0.00% <ø> (ø)`
cmd/goal/clerk.go	`8.75% <0.00%> (ø)`
cmd/goal/node.go	`12.71% <ø> (ø)`
cmd/tealdbg/localLedger.go	`64.81% <ø> (ø)`
daemon/algod/api/server/router.go	`13.88% <0.00%> (-0.82%)`	⬇️
daemon/algod/api/server/v2/handlers.go	`0.91% <0.00%> (ø)`
daemon/algod/api/server/v2/test/helpers.go	`73.94% <ø> (ø)`
daemon/algod/api/server/v2/utils.go	`12.92% <0.00%> (ø)`
libgoal/libgoal.go	`2.60% <0.00%> (ø)`
... and 20 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

…port-changes

shiqizng · 2022-11-04T16:53:45Z

daemon/algod/api/algod.oas2.json

@@ -227,6 +236,10 @@
    "/v2/accounts/{address}/assets/{asset-id}": {
      "get": {
        "description": "Given a specific account public key and asset ID, this call returns the account's asset holding and asset parameters (if either exist). Asset parameters will only be returned if the provided address is the asset's creator.",
+        "tags": [
+          "public",
+          "nonparticipating"


are these endpoints not available when nodes are running in participation mode?

shiqizng · 2022-11-04T16:55:17Z

daemon/algod/api/algod.oas2.json

@@ -381,6 +398,10 @@
    "/v2/accounts/{address}/transactions/pending": {
      "get": {
        "description": "Get the list of pending transactions by address, sorted by priority, in decreasing order, truncated at the end at MAX. If MAX = 0, returns all pending transactions.\n",
+        "tags": [
+          "public",
+          "participating"


similar to previous question, are these endpoints not available to nodes running non-part mode?

Good question! In this PR I'm just separating APIs into groups based on potential "modes" of operation for a node, but in the routers.go file I'm registering all of them as normal.

In a followup PR the plan is to implement variations of AlgorandFullNode which have different subsets of functionality turned off/on.

Currently the list I'm working with is:

Participating-- runs APIs tagged with nonparticipating and participating

NonParticipating-- runs APIs tagged with nonparticipating

Data-- runs APIs tagged with nonparticipating and data

This should allow us to more easily turn on/off the APIs based on the purpose of the node, but also it will allow us to make the node more lightweight in certain cases. For example, a non participating node would no longer run the agreement service, or have a txn pool, etc.

And in the case of the data mode, we can ensure that the user has full control over what round the node is on because the agreement service won't be running to try to advance the round. That's important when implementing sync round and delta APIs in #4658

@Eric-Warehime Based on what I understand PR scope to be, I think we can defer a decision until a subsequent PR. My intent is to raise for light consideration because we're in the neighborhood.

I'm interested in selecting a different name for data mode.

Off-the-cuff, I don't have a better suggestion (NonParticipatingDeveloper?).

I think you suggested as much in a verbal: Perhaps we can move away from EnableDeveloperAPI towards mode instead?

I'm unfamiliar with the history, but perhaps it's sensible for the endpoints guarded by EnableDeveloperAPI to be non-participating only.

If we grouped additional routes under the data umbrella, it might motivate a different name.

Ultimately, if we don't have alternative names, I'm good as is. If you feel Data works well, I also won't push further.

I'll echo @michaeldiamant question (and caveat that this is a good time to discuss, but out of scope for the PR). I'm not sure if "NonParticipatingDeveloper" makes sense, since dev-mode is a single-node instance it must be participating.

Additionally, I had some of the same confusion as Shiqi. I think the names "participating" and "nonparticipating" would be user settings that describe a set of feature, and the tags here might be clearer if written in terms of the feature.

participating - I think this is fine. There are also participation specific endpoints, so it maps well to the API.

non-participating - What is the difference between this and data (or ledger)

admin - probably a better name than public/private. Maybe we don't even need public.

w/r/t public vs. private, I'm open to changing to admin for private. The reason I liked public is that made the directory structure a bit more predictable.

For the other names, the difference between a non-participating node and a data or ledger (or whatever) node is for now solely the new set of APIs being added for retrieving deltas and managing the ledger's sync round. I think if we want to we can also include things like the v2/teal/ endpoints. Also, given the work being done for new Debugger hooks, and expanding the capability of the node to provide fine grained txn tracing, it makes sense to me to put APIs exposing that data in this mode--we probably wouldn't want Txn tracers running on a normal participating or nonparticipating node.

shiqizng · 2022-11-04T19:18:25Z

daemon/algod/api/server/v2/utils.go

 	if formatPtr != nil {
-		format = strings.ToLower(*formatPtr)
+		format = model.PendingTransactionInformationParamsFormat(*formatPtr)


*formatPtr doesn't need to be lowered cased anymore?

I added it back in the most recent commits. It looks like it got removed when the method was changed previously, but you're right we still want to be case insensitive here.

daemon/algod/api/server/v2/generated/participating/private/private_routes.yml

daemon/algod/api/algod.oas2.json

winder · 2022-11-09T14:20:16Z

daemon/algod/api/client/restClient.go

-	generatedV2 "github.com/algorand/go-algorand/daemon/algod/api/server/v2/generated"
-	privateV2 "github.com/algorand/go-algorand/daemon/algod/api/server/v2/generated/private"
-
 	"github.com/algorand/go-algorand/crypto"
+	"github.com/algorand/go-algorand/daemon/algod/api/server/v2/generated/model"


winder · 2022-11-09T14:30:47Z

daemon/algod/api/server/v2/generated/participating/private/private_routes.yml

+package: private
+generate:
+  echo-server: true
+  embedded-spec: true


This can be turned off in the config files.

Suggested change

embedded-spec: true

embedded-spec: false

Yes, we can remove it--I've left it in since it matches what we currently have.

winder · 2022-11-09T14:37:03Z

daemon/algod/api/server/router.go

-	generated.RegisterHandlers(e, &v2Handler, apiAuthenticator)
-	private.RegisterHandlers(e, &v2Handler, adminAuthenticator)
+	nppublic.RegisterHandlers(e, &v2Handler, apiAuthenticator)
+	npprivate.RegisterHandlers(e, &v2Handler, adminAuthenticator)
+	ppublic.RegisterHandlers(e, &v2Handler, apiAuthenticator)
+	pprivate.RegisterHandlers(e, &v2Handler, adminAuthenticator)


The "real" change. 👍

michaeldiamant

@Eric-Warehime Thanks for bringing in my feedback! ☕

Eric-Warehime added 17 commits October 27, 2022 10:30

Commit diff of oapi-codegen update

573a1fd

Fix git stash issues

25818d8

Rerun generation with skip-prune

e2d8c33

Rerun generate w/ GenerateEnums patch

337bf9e

Update handlers based on new Enum types

a3d3c29

Update to oapi-codegen v1.11.0-algorand.0

fda1f97

Fix type renaming/tests

f66b3c9

Further update types changed in generated code

582471f

We now ignore invalid query params

f617d87

Another AssetId=>AssetID error

0b41dc3

Update to oapi-codegen v1.12.0-algorand.0

e45b98b

Fix type changes

0009b0c

Fix test type error

d2048d6

Merge remote-tracking branch 'upstream/master' into oapi-v1.11.0-back…

27bdd45

…port-changes

Fix the rest of the box conflicts

0808134

Eric-Warehime added the Enhancement label Oct 31, 2022

Eric-Warehime requested review from winder, AlgoStephenAkiki, jasonpaulos, michaeldiamant and shiqizng October 31, 2022 19:53

Eric-Warehime added 3 commits November 1, 2022 07:52

Split APIs into participating/nonparticipating

d6d9c29

Merge remote-tracking branch 'upstream/master' into oapi-v1.11.0-back…

5cbe20f

…port-changes

Remove comment

c4a3f8a

algorandskiy assigned Eric-Warehime Nov 4, 2022

Eric-Warehime added 2 commits November 4, 2022 06:22

Merge remote-tracking branch 'upstream/master' into oapi-v1.11.0-back…

120a92b

…port-changes

Fix AssetID change

4722698

shiqizng reviewed Nov 4, 2022

View reviewed changes

Eric-Warehime mentioned this pull request Nov 4, 2022

Ledger Deltas: AccountDelta retrieval through ledger/catchup service #4658

Merged

2 tasks

Eric-Warehime added 2 commits November 7, 2022 14:45

Merge remote-tracking branch 'upstream/master' into consolidate-models

703144b

Downcase type conversions in server utils

e63a566

Eric-Warehime requested a review from algochoi November 8, 2022 00:06

Eric-Warehime changed the title ~~WIP: algod: Consolidate models~~ algod: Consolidate models Nov 8, 2022

Eric-Warehime marked this pull request as ready for review November 8, 2022 15:57

michaeldiamant reviewed Nov 8, 2022

View reviewed changes

daemon/algod/api/server/v2/generated/participating/private/private_routes.yml Show resolved Hide resolved

winder reviewed Nov 9, 2022

View reviewed changes

daemon/algod/api/algod.oas2.json Show resolved Hide resolved

winder reviewed Nov 9, 2022

View reviewed changes

winder previously approved these changes Nov 9, 2022

View reviewed changes

Eric-Warehime added 3 commits November 13, 2022 22:05

Merge remote-tracking branch 'upstream/master' into consolidate-models

fef5c8f

Add abort catchup API tags

f4bd5fa

Update code gen config tags

0171f8e

Eric-Warehime dismissed winder’s stale review via 0171f8e November 14, 2022 06:28

Merge remote-tracking branch 'upstream/master' into consolidate-models

ea80a8b

Eric-Warehime requested review from winder and michaeldiamant November 14, 2022 16:59

winder approved these changes Nov 14, 2022

View reviewed changes

michaeldiamant approved these changes Nov 14, 2022

View reviewed changes

winder merged commit 23890a8 into algorand:master Nov 14, 2022

This was referenced Nov 21, 2022

go-algorand 3.12.1-beta Release PR #4820

Closed

go-algorand 3.12.1-beta Release PR #4823

Merged

onetechnical mentioned this pull request Nov 22, 2022

go-algorand v3.12.2-beta #4828

Merged

Algo-devops-service mentioned this pull request Nov 28, 2022

go-algorand 3.12.2-stable Release PR #4837

Merged

Eric-Warehime mentioned this pull request Dec 8, 2022

algod: Remove unused generated code #4880

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

algod: Consolidate models #4714

algod: Consolidate models #4714

Eric-Warehime commented Oct 31, 2022 •

edited

Loading

codecov bot commented Oct 31, 2022 •

edited

Loading

shiqizng Nov 4, 2022 •

edited

Loading

shiqizng Nov 4, 2022

Eric-Warehime Nov 4, 2022

michaeldiamant Nov 8, 2022

winder Nov 9, 2022

winder Nov 9, 2022

Eric-Warehime Nov 9, 2022

shiqizng Nov 4, 2022

Eric-Warehime Nov 8, 2022

winder Nov 9, 2022

winder Nov 9, 2022

Eric-Warehime Nov 9, 2022

winder Nov 9, 2022

michaeldiamant left a comment

algod: Consolidate models #4714

algod: Consolidate models #4714

Conversation

Eric-Warehime commented Oct 31, 2022 • edited Loading

Summary

Test Plan

codecov bot commented Oct 31, 2022 • edited Loading

Codecov Report

shiqizng Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaeldiamant left a comment

Choose a reason for hiding this comment

Eric-Warehime commented Oct 31, 2022 •

edited

Loading

codecov bot commented Oct 31, 2022 •

edited

Loading

shiqizng Nov 4, 2022 •

edited

Loading